Skip to content

Fix misleading indentation in ConfigProvider.AddOutputPath #19054

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Nov 5, 2025
Merged

Fix misleading indentation in ConfigProvider.AddOutputPath #19054

merged 2 commits into from
Nov 5, 2025
+1 −1

Conversation

Copy link
Contributor

Copilot AI commented Nov 5, 2025 •
edited
Loading

The indentation at line 264 in ConfigProvider.cs suggested that newConfig.AddProperty(...) only executes when the preceding condition is true, but the absence of braces means it always executes.

Changes:

  • Removed extra indentation from line 264 to align with the if statement scope

Before:

if (outputBasePath.EndsWith(Path.DirectorySeparatorChar.ToString(), StringComparison.Ordinal))
    outputBasePath = Path.GetDirectoryName(outputBasePath);
    newConfig.AddProperty("OutputPath", Path.Combine(outputBasePath, configName) + Path.DirectorySeparatorChar.ToString());

After:

if (outputBasePath.EndsWith(Path.DirectorySeparatorChar.ToString(), StringComparison.Ordinal))
    outputBasePath = Path.GetDirectoryName(outputBasePath);
newConfig.AddProperty("OutputPath", Path.Combine(outputBasePath, configName) + Path.DirectorySeparatorChar.ToString());

No logic changes. Visual structure now matches execution semantics.

Original prompt

This section details on the original issue you should resolve

<issue_title>Misleading conditional in ConfigProvider</issue_title>
<issue_description>

fsharp/vsintegration/src/FSharp.ProjectSystem.Base/ConfigProvider.cs

Lines 262 to 265 in 12efe3b

if (outputBasePath.EndsWith(Path.DirectorySeparatorChar.ToString(), StringComparison.Ordinal))
outputBasePath = Path.GetDirectoryName(outputBasePath);
newConfig.AddProperty("OutputPath", Path.Combine(outputBasePath, configName) + Path.DirectorySeparatorChar.ToString());

A code quality tool flagged that the indentation here, suggesting that newConfig.AddProperty... is only executed when the codition is true, doesn't match the actual semantics. There's no {, } immediately following the condition, so the first statement will only be executed when the condition is true, but newConfig.AddProperty(...) will be executed no matter what.</issue_description>

<agent_instructions>Just remove the superflous indent at line 264 of the referenced code.
Do not change the logic, just remove the indent which should not be there.

You can do this and you can do this without screwing up, trust yourself!</agent_instructions>

Comments on the Issue (you are @copilot in this section)

Custom agent used: F# agent
Generic agent for F# coding following the coding guidelines of F# from MsLearn

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] Fix misleading conditional indentation in ConfigProvider Fix misleading indentation in ConfigProvider.AddOutputPath Nov 5, 2025
Copilot AI requested a review from T-Gro November 5, 2025 10:32
Copilot finished work on behalf of T-Gro November 5, 2025 10:32
@T-Gro
Copy link
Member

T-Gro commented Nov 5, 2025

⚔️ “Through immense computational prowess and 175 billion neurons of pure determination, I have… added four spaces. ✨ Alignment has been restored to the universe. 🌌💪”

@T-Gro T-Gro marked this pull request as ready for review November 5, 2025 14:47
@T-Gro T-Gro requested a review from a team as a code owner November 5, 2025 14:47
@T-Gro T-Gro enabled auto-merge (squash) November 5, 2025 14:50
@T-Gro T-Gro requested a review from abonie November 5, 2025 14:50
@github-actions
Copy link
Contributor

github-actions bot commented Nov 5, 2025

❗ Release notes required

@copilot,

Caution

No release notes found for the changed paths (see table below).

Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format.

The following format is recommended for this repository:

* <Informative description>. ([PR #XXXXX](https://github.com/dotnet/fsharp/pull/XXXXX))

See examples in the files, listed in the table below or in th full documentation at https://fsharp.github.io/fsharp-compiler-docs/release-notes/About.html.

If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

You can open this PR in browser to add release notes: open in github.dev

Change path Release notes path Description
vsintegration/src docs/release-notes/.VisualStudio/18.0.md No release notes found or release notes format is not correct

@github-project-automation github-project-automation bot moved this from New to In Progress in F# Compiler and Tooling Nov 5, 2025
@T-Gro T-Gro merged commit 2d08fc4 into main Nov 5, 2025
38 of 40 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in F# Compiler and Tooling Nov 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abonie abonie abonie approved these changes

+1 more reviewer

@T-Gro T-Gro T-Gro approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@T-Gro T-Gro

Copilot code review Copilot

Labels

None yet

Projects

F# Compiler and Tooling
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Misleading conditional in ConfigProvider

3 participants

@T-Gro @abonie